Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CP-44440: Collect config and logs in bugreport #22

Conversation

BengangY
Copy link
Contributor

@BengangY BengangY commented Nov 7, 2023

Collect 4 files for SNMP:
/etc/snmp/snmp.xs.conf
/etc/snmp/snmpd.xs.conf
/var/lib/net-snmp/snmpd.conf
/etc/sysconfig/snmpd

@BengangY BengangY force-pushed the private/bengangy/CP-CP-44440 branch from f699dea to 4763b49 Compare November 7, 2023 05:54
@BengangY BengangY requested a review from DeliZhangX November 7, 2023 05:55
@BengangY BengangY force-pushed the private/bengangy/CP-CP-44440 branch from 4763b49 to b86ae8c Compare November 7, 2023 06:03
lindig
lindig previously approved these changes Nov 7, 2023
xen-bugtool Outdated Show resolved Hide resolved
Copy link
Collaborator

@bernhardkaindl bernhardkaindl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't call out to run sed if you have re, see my commit suggestion for the first of these filter functions.

xen-bugtool Outdated Show resolved Hide resolved
xen-bugtool Outdated Show resolved Hide resolved
xen-bugtool Outdated Show resolved Hide resolved
Copy link
Collaborator

@bernhardkaindl bernhardkaindl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the other changes to commit (can be committed using the commit buttons, and SYSCONFIG_SNMPD = '/etc/sysconfig/snmpd'.

xen-bugtool Outdated Show resolved Hide resolved
xen-bugtool Outdated Show resolved Hide resolved
@DeliZhangX
Copy link

Change commit message to CP-44440: Collect SNMP config and logs

xen-bugtool Outdated Show resolved Hide resolved
xen-bugtool Outdated Show resolved Hide resolved
@BengangY BengangY force-pushed the private/bengangy/CP-CP-44440 branch from c199dab to 85902f0 Compare November 13, 2023 10:22
@BengangY BengangY force-pushed the private/bengangy/CP-CP-44440 branch from 85902f0 to 465259f Compare November 14, 2023 02:54
Copy link
Collaborator

@bernhardkaindl bernhardkaindl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if the consecutive empty lines in the example snmpd.conf can be removed.

PS: I thought I had seen some trailing spaces at other places in git diff but have no enough time to check it now for verification.

Copy link
Collaborator

@bernhardkaindl bernhardkaindl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit removal of trailing spaces.

tests/integration/dom0-template/etc/snmp/snmp.xs.conf Outdated Show resolved Hide resolved
tests/integration/dom0-template/etc/snmp/snmp.xs.conf Outdated Show resolved Hide resolved
tests/integration/dom0-template/etc/snmp/snmp.xs.conf Outdated Show resolved Hide resolved
tests/integration/dom0-template/etc/snmp/snmp.xs.conf Outdated Show resolved Hide resolved
@BengangY BengangY force-pushed the private/bengangy/CP-CP-44440 branch from 465259f to d47b6ae Compare November 21, 2023 01:43
DeliZhangX
DeliZhangX previously approved these changes Nov 21, 2023
Copy link
Collaborator

@bernhardkaindl bernhardkaindl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test suite log (unfold the Checks section, click on Details and then on the Python2 GitHub action test run to find it) shows that all the greps fail to find the paths that they should grep:

https://github.com/xenserver/status-report/actions/runs/6938095957/job/18873299352?pr=22

+ grep -q 'snmpv2cpublic\|0xsnmpv3authenticationkey\|0xsnmpv3privacykey' /code/status-report/.tmp/tests/snmp/tar/snmp_xs_conf.out
grep: /code/status-report/.tmp/tests/snmp/tar/snmp_xs_conf.out: No such file or directory
+ grep -q snmpv2cpublic /code/status-report/.tmp/tests/snmp/tar/snmpd_xs_conf.out
grep: /code/status-report/.tmp/tests/snmp/tar/snmpd_xs_conf.out: No such file or directory
+ grep -q '0xsnmpv3authenticationkey\|0xsnmpv3privacykey' /code/status-report/.tmp/tests/snmp/tar/snmpd_conf.out
grep: /code/status-report/.tmp/tests/snmp/tar/snmpd_conf.out: No such file or directory

Also, they should not check the presence of certain string like they attempt to do here.

Instead, they should check two things:

  1. The test should assert that the dummy secrets present in the test template files are NOT found anywhere in the archived output!
  2. The test should assert that the diff between the actual file and the filtered output is only that the secrets are replaced with REMOVED, and other lines are not changed.

This could better be implemented based on the new pytest framework to be merged with #25, where you can implement and test these assertions locally by just running pytest tests/integration -rA` and improve it directly on a Linux development machine (like the Eddie3 in the UK) or any other Linux including WSL2 instances on Windows hosts.

@BengangY BengangY force-pushed the private/bengangy/CP-CP-44440 branch from 3e54079 to 185b56d Compare November 23, 2023 10:32
@BengangY BengangY changed the base branch from master to feature/snmp-status-report November 23, 2023 10:34
@BengangY
Copy link
Contributor Author

@bernhardkaindl I created another ticket CP-46759 to add testcases for collecting SNMP config files. Please merge the current commit into the feature branch.
After I finish the testcases, I will create another PR for testcases and merge it into the feature branch.
After the 2 commits are merged into the feature branch, please merge the feature branches into the master branch.
Thanks.

Copy link
Collaborator

@bernhardkaindl bernhardkaindl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bernhardkaindl I created another ticket CP-46759 to add testcases for collecting SNMP config files. Please merge the current commit into the feature branch.
After I finish the testcases, I will create another PR for testcases and merge it into the feature branch.
After the 2 commits are merged into the feature branch, please merge the feature branches into the master branch.
Thanks.

Yep, that's perfect!
It's also a great idea to add the testcases as a 2nd, separate commit!

@bernhardkaindl bernhardkaindl merged commit 2e5a660 into xenserver:feature/snmp-status-report Nov 23, 2023
2 checks passed
@bernhardkaindl bernhardkaindl added Needs testcase to ensure it's working properly Needs testcases, for verification for it to actually work and keeps working throuh Python3 migration Of possible interest for 8.2 CU1 This might be interesting to have for 82. CU1 labels Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs testcase to ensure it's working properly Needs testcases, for verification for it to actually work and keeps working throuh Python3 migration Of possible interest for 8.2 CU1 This might be interesting to have for 82. CU1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants